New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Expose maxPaddingWidth in OutputSettings keeping default as 30 #1655
Conversation
Thanks Jeremy, this looks good. One thing: I think we should leave the existing StringUtils.padding(width) as not deprecated. And modify the comment so that it mentions it currently defaults to 30 (i.e. not part of the contract). If people are using this method currently and are happy with it, I don't feel that they need to take on the requirement to migrate to a method that allows customization. Yes, please go ahead and re-base so it only includes the code changes, not the GitHub action runners. Without it being marked deprecated, we also don't need the ignore line in the POM. BTW, I don't think that's a javacmp bug so much as an interaction with the current configuration. I have set it to ignore methods that are marked deprecated so that I could delete a set of old methods. But I guess introducing a new deprecated method that was then ignored looks like it was unexpectedly removed. |
@jhy Thanks for the review. I have rebased against your current master and then rebased my commits so only the padding is in this now. The original method did note the 30 max so I added a note there if they need more to use the other method. For the other items, I'll save those for a separate set of pull requests later. Thanks. |
And added a test that the max level set in OutputSettings is applied.
Thanks, merged! I made a small change to make sure that the maximum is applied before the memo check, in case one wants a maximum < 20. Also added a testcase for the OutputSettings to make sure those methods are actually rigged up. |
Great! Thanks!
Sent from my Verizon, Samsung Galaxy smartphone
Get Outlook for Android<https://aka.ms/AAb9ysg>
…________________________________
From: Jonathan Hedley ***@***.***>
Sent: Wednesday, October 20, 2021 10:51:44 PM
To: jhy/jsoup ***@***.***>
Cc: Jeremy Landis ***@***.***>; Author ***@***.***>
Subject: Re: [jhy/jsoup] Expose maxPaddingWidth in OutputSettings keeping default as 30 (#1655)
Thanks, merged! I made a small change to make sure that the maximum is applied before the memo check, in case one wants a maximum < 20. Also added a testcase for the OutputSettings to make sure those methods are actually rigged up.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub<#1655 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AAHODI44B2NENBCL5A4TE53UH554BANCNFSM5GEHSC2A>.
Triage notifications on the go with GitHub Mobile for iOS<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
Expose maxPaddingWidth in OutputSettings keeping default as 30 (jhy#1655)
Fixes #1653
In order to get my fork up and going, I updated github actions here. I removed jdk 16, added jdk 18-ea, and moved to zulu distribution as temurin does not have 18-ea.
For this change, I added a changelog, added some clarification to 'memoised' padding, deprecated original padding call by having it call newer one with exposed maxPaddingWidth, skipped the min check with -1, and finally called the new one. In an attempt to better understand 'memoised' logic, I actually added more tests than necessary to the existing one for clarity purposes on how that method behaves.
The overall change though tripped up the japicmp plugin. It kept saying the method had been removed. If I removed the deprecation markings I added, it was happy. So I think that plugin only really works against deprecations from prior version. I tried a few different ways to make that work and finally gave up so I just added the class to ignore for the time being. I believe that is a bug in their plugin and that deprecation is important if anyone is otherwise externally using that method since it directs them on what to update.
One other thing, on my fork, github no longer auto turns on actions so I had to get that working so I hunted for some basic items to get it going. I noticed that jetty-servlet was not updated. That is odd given dependabot usage and the other module from jetty was updated so I bumped it up.
Obviously this has more than what I should have for a new pull request. For purposes of review, please use this. If you do need me to then rebase and only send up direct change portions, no problem, I can rebase as necessary.
Thanks,
Jeremy